Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix no common properties when logging error by aligning 'properties' contract #193208

Closed
wants to merge 7 commits into from
Closed

Fix no common properties when logging error by aligning 'properties' contract #193208

wants to merge 7 commits into from

Conversation

Joouis
Copy link

@Joouis Joouis commented Sep 15, 2023 •
edited
Loading

  • Aossicated issue: Missing common properties in extension telemetry when logging error event Missing common properties in extension telemetry when logging error event #193205
  • Ensure that the code is up-to-date with the main branch: will update the branch after approvals
  • Description:
    • Please check the issue mentioned above firstly to understand the motivation of this PR.
    • The root cause of missing common properties when logging error is that the data structure is not uniform when mixing common props in. No matter "properties" is in or not in data object, I think the function should keep the same structure for the downstream functions instead of two different looks. The telemetry extension follows the API contract from App Insights package to pass "properties" and "measurements" accordingly, if common properties are not in the "properties" then the bug triggered.
    • This is the expected log screenshot by this change.
      image

@Joouis
Copy link
Author

Joouis commented Sep 15, 2023

It's strange that telemetry will not be sent in Code - OSS, the development way is like open Code - OSS by yarn watch, then open my extension project by Code - OSS process, debug my extension to open another Code -OSS process. One VSCode and two Code - OSS windows opened at same time, this could be a little bit complex. And my workaround steps are:

  1. Throw an error in my extension code and print the data from mixInCommonPropsAndCleanData() in Code - OSS
  2. Starting the debug mode of VSCode and set break point at the beginning of sendErrorData() internally
  3. Throw an error in my extension code again to trigger the break point, then replace the data value passed to sendErrorData() by the printed data in step 1.

Finally I can check the data on App Insights. @lramos15 any idea for this? Thanks :)

@lramos15
Copy link
Member

Seems like the CI is failing due to telemetry tests failing

@Joouis
Copy link
Author

Joouis commented Sep 15, 2023

Yes I'm looking into it.

@aeschli aeschli assigned lramos15 and unassigned aeschli Sep 21, 2023
@Joouis
Copy link
Author

Joouis commented Oct 12, 2023

Hi @aeschli, I haven't heard back from Logan in two weeks, could you help on the PR? I think #193205 affects all customers that no common properties for "unhandlederror" type, making anlysis harder like from which extension version the bug begins.

@aeschli
Copy link
Contributor

aeschli commented Oct 12, 2023

@Joouis Sorry, we need Logan for this..

@Joouis
Copy link
Author

Joouis commented Oct 12, 2023

So kindly ping @lramos15 again.

@lramos15
Copy link
Member

So kindly ping @lramos15 again.

I'm aware of this PR, but am a bit swamped at the moment with some other work. Thank you for your patience 🙏

@lramos15 lramos15 mentioned this pull request Nov 15, 2023
Merged
@Joouis Joouis closed this Jan 19, 2024
@Joouis Joouis deleted the joou/fix-missing-common-props-for-unhandlederror branch January 19, 2024 06:49
@microsoft microsoft locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lramos15 lramos15 lramos15 left review comments

Assignees

@lramos15 lramos15

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Joouis @lramos15 @aeschli